Fix/arbitrary failing on feature flag by razzat008 · Pull Request #1128 · Devolutions/IronRDP

rajatdahal (razzat008) · 2026-02-26T16:53:16Z

working on #1121

This commit adds conditional attribute, which implements the `Arbitrary` trait when compiled with `--features arbitrary`; this is to ensure, sucessful checks/builds for future fuzzing implementation

…re_flag

rajatdahal (razzat008) · 2026-02-26T16:56:39Z

Benoît Cortier (@CBenoit)
Some structs(inside bitflags macro) needed manual implementation of the Arbitrary trait, so I'll push them too shortly.

Added conditional arbitrary for structs/enums that support it; files related to this commit most probably won't require manual impl of the Arbitrary trait. Due to inter-dependency, most of the struct/enums had to be configured

…uence The mannual impl for ChannelName follows the 8-bi null-terminated sequence; and the mannual impl for LicenseExchangeSequence generates arbitrary for all attributes except the license_cache

#cfg_attr for ChunkProcessor,StaticVirtualChannel and a constructor for the StaticChannelSet(as it has TypeId which is populated by the compiler)

rajatdahal (razzat008) · 2026-02-27T03:16:34Z

I was not confident with some of the impls so I've added MIGHT NEED CHANGE for those parts.
Also, the StaticChannelSet had an attribute which used the TypeId which didn't implement the Arbitrary trait, so it just calls the constructor for now.

Mannual impls are done for:

LicenseExchangeSequence: the license_cache attribute had dyn trait
ChannelName: a valid null terminated sequence was required

rajatdahal (razzat008) · 2026-02-27T03:18:32Z

The checks are passing for as of now:

cargo check -p ironrdp-connector --features arbitrary
cargo check -p ironrdp-pdu --features arbitrary

Benoît Cortier (CBenoit) · 2026-02-27T05:32:00Z

    pub domain: Option<String>,
    pub hardware_id: [u32; 4],
    pub license_cache: Arc<dyn LicenseCache>,
+    pub _phantom: std::marker::PhantomData<u8>,


question: Why did you add this PhantomData?

I initially thought it was required for the dyn trait object to compile successfully, so I left it in there and forgot about it, even after the manual impl for LicenseExchangeSequence.
It compiles fine without it and isn't needed, my bad:))

Benoît Cortier (CBenoit) · 2026-02-27T05:35:31Z

+            // MIGHT NEED CHANGE, currently it's not using arbitrary function
+            license_cache: Arc::new(NoopLicenseCache),


I think this is perfectly fine as we don’t provide any LicenseCache implementation as part of this crate. Implementers wishing to fuzz using their license cache should do so on their side.

Benoît Cortier (CBenoit)

I’m not entirely sure what the Arbitrary trait is meant to do in ironrdp-svc: we’re probably not going to deal with "arbitrary static virtual channels", so maybe a better way would be to manually implement the trait at some places more in ironrdp-connector 🤔

rajatdahal (razzat008) · 2026-02-27T12:12:44Z

I understand, the structs in the ironrdp-svc crate were used in some other places which had to implement arbitrary, so without those checks wouldn't compile.
I will look into manual implementation for the connector.
Thanks

Greg Lamberson (glamberson) · 2026-05-12T02:10:31Z

Hi rajatdahal (@razzat008),

Thanks for taking this on. The arbitrary feature is a meaningful unblocker for the fuzzing roadmap Benoît Cortier (@CBenoit) outlined in #1120 and the four sub-issues stacked under it.

I have been reading through the diff in preparation for picking up some of the structured-fuzzing follow-on work, and I wanted to consolidate the open items in one place so this PR has a clear path to land.

Outstanding technical items

Architectural placement of Arbitrary in ironrdp-svc. Benoît Cortier (@CBenoit)'s last review suggested moving these impls into manual implementations in ironrdp-connector instead, since ironrdp-svc is unlikely to need fuzz support directly. This is the substantive directional change still pending.
StaticChannelSet Arbitrary constructor fallback. The current Ok(StaticChannelSet::new()) approach produces only the empty channel set under fuzzing, which means the connector state machine never sees a populated channel set during structured fuzz runs. If the intent is to exercise channel-dependent code paths under fuzz, this would need to populate the set with some plausible channels. If not, leaving it as a documented limitation in the PR description is fine.
Option<Arc<dyn LicenseCache>> using #[arbitrary(default)]. Same trade-off shape as item 2: license-cache-dependent paths never execute under fuzz with this approach. Worth recording the decision in the PR description either way.
PhantomData in license_exchange.rs. You already acknowledged in the thread that this is unnecessary. Worth removing in the next push.
The // MIGHT NEED CHANGE markers. Useful to keep visible during review, but should be resolved before merge.
Feature flag wiring for no_std + alloc. The umbrella in Fix arbitrary feature flag plumbing so it compiles #1121 specifies the wiring should be arbitrary = ["alloc", "dep:arbitrary"] for crates that support no_std + alloc. The current wiring in ironrdp-connector/Cargo.toml is arbitrary = ["dep:arbitrary", "ironrdp-pdu/arbitrary"], which omits the alloc feature dependency. Worth checking whether ironrdp-connector actually supports no_std + alloc (it looks std-only on a quick read) and adjusting the feature line if needed.
Rebase against master and cargo fmt. The branch is in CONFLICTING state due to drift since February, and the format check is failing. These would need to be cleared.

Coordination

If you are still actively working on this and just bandwidth-limited, that is fine on my side. If you would welcome a collaborator on the architectural reshape in item 1 and the rebase in item 7 while you stay as the main author, I can chip in there. Either way works for me. The structured-fuzzing infrastructure work in #1122 and #1124 is downstream of this PR landing.

Let me know what would be most useful.

Greg Lamberson (glamberson) · 2026-05-13T23:23:15Z

Hi rajatdahal (@razzat008),

I went ahead and filed #1272 to address #1121 directly. The downstream fuzzing work in #1122 through #1124 was blocked on this landing, and the 2026-05-11 ping here did not get a response, so I treated the path as clear to step in.

Your exploration in this PR was the starting point. The decisions in #1272 about the ChannelName manual impl shape, the Arc<dyn LicenseCache> trade-off, the StaticChannelSet skip, and the feature-cascade wiring all build on the questions you and Benoît Cortier (@CBenoit) raised in this thread. Thanks for putting the original work in.

Benoît Cortier (CBenoit) · 2026-05-14T13:03:39Z

Closing this PR since #1272 was merged earlier today.

Thank you for the initial exploration!

rajatdahal (razzat008) added 2 commits February 26, 2026 22:34

fix(fuzzing): add arbitrary support for future fuzzing compatibility

36a66d5

This commit adds conditional attribute, which implements the `Arbitrary` trait when compiled with `--features arbitrary`; this is to ensure, sucessful checks/builds for future fuzzing implementation

Merge branch 'Devolutions:master' into fix/arbitrary_failing_on_featu…

eca902e

…re_flag

rajatdahal (razzat008) added 3 commits February 27, 2026 08:31

add(cfg_attr): arbitrary feature cfg

bbd64db

Added conditional arbitrary for structs/enums that support it; files related to this commit most probably won't require manual impl of the Arbitrary trait. Due to inter-dependency, most of the struct/enums had to be configured

add(cfg_attr): mannual arbitrary for ChannelName & LicenseExchangeSeq…

28849e5

…uence The mannual impl for ChannelName follows the 8-bi null-terminated sequence; and the mannual impl for LicenseExchangeSequence generates arbitrary for all attributes except the license_cache

add(cfg_attr): arbitrary feature cfg

540b4fa

#cfg_attr for ChunkProcessor,StaticVirtualChannel and a constructor for the StaticChannelSet(as it has TypeId which is populated by the compiler)

Benoît Cortier (CBenoit) reviewed Feb 27, 2026

View reviewed changes

removed: _phantom fields as they're not needed

3da0c13

This was referenced May 13, 2026

feat(fuzz): add egfx PDU decoders to pdu_decode oracle + fix two surfaced bugs #1271

Open

feat(pdu): add arbitrary feature for structure-aware fuzzing #1272

Merged

Greg Lamberson (glamberson) mentioned this pull request May 13, 2026

Advanced fuzzing with arbitrary #1120

Open

Benoît Cortier (CBenoit) closed this May 14, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/arbitrary failing on feature flag#1128

Fix/arbitrary failing on feature flag#1128
rajatdahal (razzat008) wants to merge 6 commits into
Devolutions:masterfrom
razzat008:fix/arbitrary_failing_on_feature_flag

rajatdahal (razzat008) commented Feb 26, 2026 •

edited

Loading

Uh oh!

rajatdahal (razzat008) commented Feb 26, 2026

Uh oh!

rajatdahal (razzat008) commented Feb 27, 2026

Uh oh!

rajatdahal (razzat008) commented Feb 27, 2026

Uh oh!

Benoît Cortier (CBenoit) Feb 27, 2026

Uh oh!

rajatdahal (razzat008) Feb 27, 2026

Uh oh!

Benoît Cortier (CBenoit) Feb 27, 2026 •

edited

Loading

Uh oh!

rajatdahal (razzat008) Feb 27, 2026

Uh oh!

Benoît Cortier (CBenoit) left a comment

Uh oh!

rajatdahal (razzat008) commented Feb 27, 2026

Uh oh!

Greg Lamberson (glamberson) commented May 12, 2026

Uh oh!

Greg Lamberson (glamberson) commented May 13, 2026

Uh oh!

Benoît Cortier (CBenoit) commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

		// MIGHT NEED CHANGE, currently it's not using arbitrary function
		license_cache: Arc::new(NoopLicenseCache),

Conversation

rajatdahal (razzat008) commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajatdahal (razzat008) commented Feb 26, 2026

Uh oh!

rajatdahal (razzat008) commented Feb 27, 2026

Uh oh!

rajatdahal (razzat008) commented Feb 27, 2026

Uh oh!

Benoît Cortier (CBenoit) Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

rajatdahal (razzat008) Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatdahal (razzat008) Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

rajatdahal (razzat008) commented Feb 27, 2026

Uh oh!

Greg Lamberson (glamberson) commented May 12, 2026

Outstanding technical items

Coordination

Uh oh!

Greg Lamberson (glamberson) commented May 13, 2026

Uh oh!

Benoît Cortier (CBenoit) commented May 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

rajatdahal (razzat008) commented Feb 26, 2026 •

edited

Loading

Benoît Cortier (CBenoit) Feb 27, 2026 •

edited

Loading